Skip to content

fix: Align all 0x34 and 0x36 framing bytes - #848

Merged
gijzelaerr merged 2 commits into
gijzelaerr:masterfrom
bvanelli:fix/align-all-0x34-byte-payloads
Sep 2, 2026
Merged

fix: Align all 0x34 and 0x36 framing bytes#848
gijzelaerr merged 2 commits into
gijzelaerr:masterfrom
bvanelli:fix/align-all-0x34-byte-payloads

Conversation

@bvanelli

Copy link
Copy Markdown
Contributor

Changes

Aligns the transmission with the received bytes when it comes to the transport flags. The C# library has the following definitions:

reference request class TransportFlags
InitSslRequest.cs:23 0x30
CreateObjectRequest.cs:23,57 0x36
DeleteObjectRequest.cs:23 0x34
ExploreRequest.cs:24 0x34 (with a // or 0x36??? comment)
GetMultiVariablesRequest.cs:24 0x34
GetVarSubstreamedRequest.cs:23 0x34
SetMultiVariablesRequest.cs:24 0x34
SetVariableRequest.cs:23 0x34

I aligned based on the testing with a S71500 PLC, and it seems to accept the framing 0x34 or 0x36 in most cases, but sometimes the wrong frame leads to a connection reset. Here are the sent and received frames:

request tx before tx now rx
InitSsl 0x30 0x30 0x70
CreateObject 0x36 0x36 0x36
SetMultiVariables 0x34 0x34 0x34
GetVarSubStreamed 0x36 0x34 0x34
Explore 0x34 0x34 0x34
DeleteObject 0x36 0x34 0x34

Once the new codes from the C# library are added, they can be added to the same frozenset to keep compatibility.

PS: I checked if this was the cause why #820 was not working, but that was not the issue.

@gijzelaerr gijzelaerr left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The transport-flag mapping is protocol behavior and currently has no regression coverage in this PR. Please add byte-level tests for both sync and async request builders that demonstrate:

  • every function in FLAGS_34_FUNCTION_CODES emits 0x34;
  • an ordinary CreateObject remains 0x36;
  • session-key requests retain their existing 0x34 override;
  • _delete_session() emits 0x34 in both clients.

The hardware result is valuable, but without these tests a later request-builder refactor can silently restore the framing mismatch. This is also a prerequisite for #849, whose capture fixtures currently fail without this branch, so the boundary needs to be locked down before merge.

@bvanelli

bvanelli commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

I have pushed 1946bd6 to test regressions in the wire format from what you mentioned, but testing there is still a little bit awkward because the payloads are generated in-line, and also not shared between sync and async library. This refactor belongs in a different PR, so I included the basic testing with the OSError trick. This can be enhanced in the future with real traces.

@gijzelaerr gijzelaerr left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The transport-flag mapping now has byte-level coverage for session CreateObject, SessionKey overrides, and sync/async DeleteObject, with the shared function-code set covering the common request builders. I ran the S7CommPlus framing and TLS suites locally: 93 passed.

@gijzelaerr
gijzelaerr merged commit 2ad082d into gijzelaerr:master Sep 2, 2026
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants